Batch asset invalidations #11937
Batch asset invalidations #11937
Conversation
…tion to the backend is in flight
| @@ -224,7 +219,7 @@ export default function DriveBar(props: DriveBarProps) { | |||
| <ConfirmDeleteModal | |||
| actionText={getText('allTrashedItemsForever')} | |||
| doDelete={() => { | |||
There was a problem hiding this comment.
Shouldn't it be a promise here?
There was a problem hiding this comment.
good point. i was being silly and figured it shouldn't be a promise because the component doesn't take a promise...
There was a problem hiding this comment.
I found doDelete is passed directly to the Form component, so we can use promises, as the form will do all the magic ✨
| @@ -503,6 +487,10 @@ export default class ProjectManager { | |||
|
|
|||
| /** Delete a project. */ | |||
| async deleteProject(params: Omit<DeleteProjectParams, 'projectsDirectory'>): Promise<void> { | |||
| const cached = this.internalProjects.get(params.projectId) | |||
There was a problem hiding this comment.
This should be avoided in the future - cache management is purely the tanstack query responsibility.
There was a problem hiding this comment.
honestly yeah. but i think this is more or less a workaround for issues in the engine API, the alternative is we'd have to implement localbackend through tanstack query instead somehow.
specifically: the "cache" here is so that we actually remember the state of the projects because (iirc?) there is no way to actually get the state of the projects though the Project Manager... if we could then none of this would be necessary
There was a problem hiding this comment.
But we have the same cache in the tanstack query, haven't we?
| } | ||
| case backend.AssetType.file: { | ||
| const details = await this.getFileDetails(asset.id, title, true) | ||
| invariant(details.url != null, 'The download URL of the file must be present.') |
There was a problem hiding this comment.
I think here would be better to throw errors/reject promise instead.
See: "You're fine throwing a descriptive error here because it's just very unlikely this will ever happen and even if it does you wouldn't really know what to do about it anyway."
There was a problem hiding this comment.
is there a reason we don't just switch out the invariant package to get this for free?
There was a problem hiding this comment.
if we switch to @epic-web/invariant, we get:
You're fine throwing a descriptive error here
for free, right? the issue with the current tiny-invariant package is that it strips out error messages
both packages are just small wrappers around throwing errors anyway, right?
There was a problem hiding this comment.
Well, possibly, yeah. Tiny invariant strips messages only for kb savings:
I think for us it's not a big deal...
|
🚫 QA:
CleanShot.2025-01-07.at.16.42.59.mp4
CleanShot.2025-01-07.at.16.50.30.mp4
|
just for delete/restore? or for move and copy as well? |
whoops... how were we avoiding re-renders previously? |
AFAIR we didn't pass the visibility object to every asset. You can use Profiler in the devtools to see what props/hooks trigger changes |
I think yes |
yes as in all of the above? |
I think that any mutation that affects assets state should invalidate all 'listDirectory` queries (both active and inactive) |
|
@somebody1234 Re-renders issue seems like not fixed :( |
|
Seems like the regression caused by un-memoized columns list |
yeah my bad, i had fixed it before and verified it, but then it re-regressed because i was too trigger-happy with removing so it turns out react compiler doesn't quite work right sometimes. i tried removing |
|
@somebody1234 it did memoize
|
|
QA ✅ |
Pull Request Description
Codebase changes:
AssetEvents andAssetListEvents. Remaining events have been moved to TanStack Query mutations in this PR, as it is neccessary for batch invalidation functionality.keyanddirectoryKeyfromAssetTreeNode, andkeys in general in favor ofids. Not strictly necessary, but it was causing logic issues and is (IMO) a nice simplification to be able to do.Important Notes
None
Testing instructions
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.
If applicable, it is suggested to paste a link to a successful run of the Extra Tests.